From d57273700bb1fd5f407028484206e17b06c7ccc2 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Gerg=C5=91=20Tisza?= Date: Tue, 27 May 2014 21:16:40 +0000 Subject: [PATCH] Add global limit to PoolCounter Adds a 'slot' option to $wgPoolCounterConf. When this option is set, there are a limited number of slots for the given worker type, regardless of key. Workers with the same key are still limited by the 'workers' option. The global limit is implemented by simply using a deterministic hash of the key instead of the real key. This avoids deadlocks, but results in slot underallocation due to hash collisions - even when there are significantly more jobs that slots, some of the slots might remain empty. Bug: 65691 Change-Id: Ibdf8ec222f9756d70de2bed7ff14913351dc394b --- includes/poolcounter/PoolCounter.php | 32 ++++++++- .../includes/poolcounter/PoolCounterTest.php | 72 +++++++++++++++++++ 2 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 tests/phpunit/includes/poolcounter/PoolCounterTest.php diff --git a/includes/poolcounter/PoolCounter.php b/includes/poolcounter/PoolCounter.php index 34953c0cda..f8d48cc707 100644 --- a/includes/poolcounter/PoolCounter.php +++ b/includes/poolcounter/PoolCounter.php @@ -53,8 +53,15 @@ abstract class PoolCounter { /** @var string All workers with the same key share the lock */ protected $key; - /** @var int Maximum number of workers doing the task simultaneously */ + /** @var int Maximum number of workers working on tasks with the same key simultaneously */ protected $workers; + /** + * Maximum number of workers working on this task type, regardless of key. + * 0 means unlimited. Max allowed value is 65536. + * The way the slot limit is enforced is overzealous - this option should be used with caution. + * @var int + */ + protected $slots = 0; /** @var int If this number of workers are already working/waiting, fail instead of wait */ protected $maxqueue; /** @var float Maximum time in seconds to wait for the lock */ @@ -66,10 +73,17 @@ abstract class PoolCounter { * @param string $key */ protected function __construct( $conf, $type, $key ) { - $this->key = $key; $this->workers = $conf['workers']; $this->maxqueue = $conf['maxqueue']; $this->timeout = $conf['timeout']; + if ( isset( $conf['slots'] ) ) { + $this->slots = $conf['slots']; + } + + if ( $this->slots ) { + $key = $this->hashKeyIntoSlots( $key, $this->slots ); + } + $this->key = $key; } /** @@ -121,6 +135,20 @@ abstract class PoolCounter { * @return Status value is one of Released/NotLocked/Error */ abstract public function release(); + + /** + * Given a key (any string) and the number of lots, returns a slot number (an integer from the [0..($slots-1)] range). + * This is used for a global limit on the number of instances of a given type that can acquire a lock. + * The hashing is deterministic so that PoolCounter::$workers is always an upper limit of how many instances with + * the same key can acquire a lock. + * + * @param string $key PoolCounter instance key (any string) + * @param int $slots the number of slots (max allowed value is 65536) + * @return int + */ + protected function hashKeyIntoSlots( $key, $slots ) { + return hexdec( substr( sha1( $key ), 0, 4 ) ) % $slots; + } } // @codingStandardsIgnoreStart Squiz.Classes.ValidClassName.NotCamelCaps diff --git a/tests/phpunit/includes/poolcounter/PoolCounterTest.php b/tests/phpunit/includes/poolcounter/PoolCounterTest.php new file mode 100644 index 0000000000..2d31d088ee --- /dev/null +++ b/tests/phpunit/includes/poolcounter/PoolCounterTest.php @@ -0,0 +1,72 @@ + 'PoolCounterMock', + 'timeout' => 10, + 'workers' => 10, + 'maxqueue' => 100, + ); + + $poolCounter = $this->getMockBuilder( 'PoolCounterAbstractMock' ) + ->setConstructorArgs( array( $poolCounterConfig, 'testCounter', 'someKey' ) ) + // don't mock anything - the proper syntax would be setMethods(null), but due to a PHPUnit bug that + // does not work with getMockForAbstractClass() + ->setMethods( array( 'idontexist' ) ) + ->getMockForAbstractClass(); + $this->assertInstanceOf( 'PoolCounter', $poolCounter ); + } + + public function testConstructWithSlots() { + $poolCounterConfig = array( + 'class' => 'PoolCounterMock', + 'timeout' => 10, + 'workers' => 10, + 'slots' => 2, + 'maxqueue' => 100, + ); + + $poolCounter = $this->getMockBuilder( 'PoolCounterAbstractMock' ) + ->setConstructorArgs( array( $poolCounterConfig, 'testCounter', 'key' ) ) + ->setMethods( array( 'idontexist' ) ) // don't mock anything + ->getMockForAbstractClass(); + $this->assertInstanceOf( 'PoolCounter', $poolCounter ); + } + + public function testHashKeyIntoSlots() { + $poolCounter = $this->getMockBuilder( 'PoolCounterAbstractMock' ) + // don't mock anything - the proper syntax would be setMethods(null), but due to a PHPUnit bug that + // does not work with getMockForAbstractClass() + ->setMethods( array( 'idontexist' ) ) + ->disableOriginalConstructor() + ->getMockForAbstractClass(); + + $hashKeyIntoSlots = new ReflectionMethod($poolCounter, 'hashKeyIntoSlots' ); + $hashKeyIntoSlots->setAccessible( true ); + + + $keysWithTwoSlots = $keysWithFiveSlots = array(); + foreach ( range( 1, 100 ) as $i ) { + $keysWithTwoSlots[] = $hashKeyIntoSlots->invoke( $poolCounter, 'key ' . $i, 2 ); + $keysWithFiveSlots[] = $hashKeyIntoSlots->invoke( $poolCounter, 'key ' . $i, 5 ); + } + + $this->assertArrayEquals( range( 0, 1 ), array_unique( $keysWithTwoSlots ) ); + $this->assertArrayEquals( range( 0, 4 ), array_unique( $keysWithFiveSlots ) ); + + // make sure it is deterministic + $this->assertEquals( + $hashKeyIntoSlots->invoke( $poolCounter, 'asdfgh', 1000 ), + $hashKeyIntoSlots->invoke( $poolCounter, 'asdfgh', 1000 ) + ); + } +} -- 2.20.1